config: set `$LESS` and `$LESSCHARSET` even if `$PAGER` is set #3657

martinvonz · 2024-05-09T22:50:13Z

Our current default for ui.pager is this:

ui.pager = { command = ["less", "-FRX"], env = { LESSCHARSET = "utf-8" } }

If the user has $PAGER set, we take that value and replace the above table by a scalar set to the value from the environment variable. That means that anyone who has set $PAGER to just less will lose both the -FRX and the charset, making e.g. colored output from jj result in escaped ANSI codes rendered by less. The lack of those options might not matter for other tools they use so they might not have realized that they wanted those options.

This patch attempts to improve the situation by changing the default ui.pager to pass -FRX via $LESS, and by setting the value from $PAGER in ui.pager.command, so the rest of the config is left alone.

The default config will still be ignored if you set the scalar ui.pager to e.g. less, since that overrides the table.

Closes #2926

Checklist

If applicable:

I have updated CHANGELOG.md
I have updated the documentation (README.md, docs/, demos/)
I have updated the config schema (cli/src/config-schema.json)
I have added tests to cover my changes

martinvonz · 2024-05-09T22:51:23Z

cli/src/config.rs

+                config::Value::new(
+                    None,
+                    config::ValueKind::Array(vec![config::Value::new(
+                        None,
+                        config::ValueKind::String(value),
+                    )]),
+                ),


This is pretty ugly. Is there a more convenient API in the config crate?

.set_override("ui.pager.command", vec![value]) appears to work.
https://docs.rs/config/0.13.4/config/enum.ValueKind.html#impl-From%3CVec%3CT%3E%3E-for-ValueKind

Actually, I'm not sure this is correct if PAGER contains a command with arguments, e.g. PAGER="less -FRX".

I'm not sure whether that is considered a kosher value for PAGER, but it usually worked for most programs, I think. Something did make me switch to using PAGER=less and LESS=... for the options, though.

It's possible this is still worth doing even if it doesn't work for PAGER="less -FRX", I'm unsure how common that is.

Or, we could allow ui.pager.command to be a string (perhaps in a future PR).

Actually, I'm not sure this is correct if PAGER contains a command with arguments, e.g. PAGER="less -FRX"?

Good point. PAGER needs to be parsed as if it were executed by the shell. It might be simpler to just special case PAGER=less.

Good idea to compare to Git. I found

https://github.com/git/git/blob/8f8d6eee531b3fa1a8ef14f169b0cb5035f7a772/Makefile#L357

Not sure what LV is about, but just following what they do seems OK to me.

Update: LV is about https://linux.die.net/man/1/lv. Haven't heard of it before. It seems obsolete (?), though it is still available in Debian.

I think git sets it regardless of PAGER, anytime LESS is unset. So, it'd again break PAGER="less -RX".

But I somehow can't reproduce this. I unset LESS, core.pager seems unset, but PAGER="less -R" git diff acts differently from PAGER="less -R +F" git diff. OTOH, PAGER="less +R" git diff prints color. So, I don't know what's going on with Git.

Update: Never mind, I think this matches the advertised Git behavior. BTW, it's documented in https://git-scm.com/docs/git-config#Documentation/git-config.txt-corepager.

I suppose if Git breaks "less -RX", we can too. The user can override it with "less -RX +F". At least, at the moment, copying Git seems like the easiest solution. We could be more conservative and only set LESS=R, but maybe it's not worth it.

Re @yuja 's options from https://github.com/martinvonz/jj/pull/3657/files#r1833607331, I think setting LESS whenever it is unset would be between 2 and 3.

Re @yuja 's options from https://github.com/martinvonz/jj/pull/3657/files#r1833607331, I think setting LESS whenever it is unset would be between 2 and 3.

yes, it would be relatively easy because we just need to update Ui::new_paged().

In my list, 1-3 would be handled in config layer. 4 needs more context.

cli/src/config/misc.toml

@ilyagr

This lets you configure e.g. some environment variables to pass to a pager that will only be used if they're not already set in the user's environment. Thanks to @ilyagr for the suggestion.

Our current default for `ui.pager` is this: ```toml ui.pager = { command = ["less"], env_default = { LESS = "-FRX", LESSCHARSET = "utf-8" } } ``` If the user has `$PAGER` set, we take that value and replace the above table by a scalar set to the value from the environment variable. That means that anyone who has set `$PAGER` to just `less` will lose both the `-FRX` and the charset, making e.g. colored output from `jj` result in escaped ANSI codes rendered by `less`. The lack of those options might not matter for other tools they use so they might not have realized that they wanted those options. This patch attempts to improve the situation by setting the value from `$PAGER` in `ui.pager.command` so the rest of the config is left alone. The default config will still be ignored if you set the scalar `ui.pager` to e.g. `less`, since that overrides the table. Closes #2926

yuja · 2024-05-10T05:40:36Z

cli/src/config.rs

+                config::Value::new(
+                    None,
+                    config::ValueKind::Array(vec![config::Value::new(
+                        None,
+                        config::ValueKind::String(value),
+                    )]),
+                ),


.set_override("ui.pager.command", vec![value]) appears to work.
https://docs.rs/config/0.13.4/config/enum.ValueKind.html#impl-From%3CVec%3CT%3E%3E-for-ValueKind

yuja · 2024-05-10T05:47:22Z

cli/src/config/misc.toml

@@ -13,7 +13,7 @@ allow-filesets = false
 always-allow-large-revsets = false
 diff-instructions = true
 paginate = "auto"
-pager = { command = ["less", "-FRX"], env = { LESSCHARSET = "utf-8" } }
+pager = { command = ["less"], env_default = { LESS="-FRX", LESSCHARSET = "utf-8" } }


Isn't it better to override the LESS variable as before?

I don't remember exactly, but some default-installed ~/.bashrc might set LESS=... If it didn't contain -R, jj's default would break.

Firstly, I think you don't need the - in LESS.

I'm not sure about Yuya's point. I have LESS set to FRX --mouse --wheel-lines=2, and I wouldn't like it overridden entirely. Perhaps we could print a warning after less quits if FRX was not in LESS and it was set? Perhaps we could prepend FRX to the variable?

I also think that, while it might make sense to add -R to LESS, forcibly adding -F (and possibly also -X) might be wrong if the user explicitly did not include them. For example, let's say we launch jj from a file manager like Midnight Commander or ranger. The user might then be relying on less not quitting after one screen to see the output at all. If we forcibly add -F, that would break this use-case. Forcibly adding -X might break it too, but I'm less sure.

So, in my opinion, if we want to hack around weird default values for LESS, we could do the analogue of LESS="R $LESS", but probably no more than that.

I now think that if some default ~/.bashrc sets LESS to some value that doesn't contain R, that's a bug in that default config. I don't think it happens often in practice.

So, it sounds like I'm converging to something that's very close to this PR as is. (See also https://github.com/martinvonz/jj/pull/3657/files#r1833620765).

One thing I'd now do differently is to move pager.env_default to pager_env_default, outside of pager, so ui.pager is a string again (by default, at least). If we set these environment variables whenever they are unset, it might be more consistent to set them even if the user sets ui.pager="less" on Windows. But the difference between that and the current behavior of the PR is not so great, it's more of a minor detail.

ilyagr · 2024-05-10T05:36:44Z

cli/src/config.rs

@@ -533,21 +533,18 @@ mod tests {

    #[test]
    fn test_command_args() {
+        let config_text = r#"


Optional: I'd call this config_toml.

ilyagr · 2024-05-10T05:43:56Z

cli/src/config.rs

+                env,
+                command,
+            } => {
+                for (k, v) in env_default {


Optional: Perhaps k.=v would work here? Or k~=v? Or (k=v)?

ilyagr · 2024-05-10T05:49:03Z

cli/src/config.rs

@@ -590,10 +610,24 @@ with_env = { env = { KEY1 = "value1", KEY2 = "value2" }, command = ["emacs", "-n
        assert_eq!(name, "emacs");
        assert_eq!(args, ["-nw"].as_ref());

+        let command_args: CommandNameAndArgs = config.get("with_env_default").unwrap();


Optional: I was going to suggest adding a test with https://doc.rust-lang.org/std/env/fn.set_var.html here, but I'm not so sure after reading the safety warning there. cargo-nextest is, in principle, multithreaded.

You could also set up a test that sets some variable that very likely exists, like PATH or CARGO. I guess to make the test reliable, you could check if that variable is set first, and skip the test if it isn't.

ilyagr · 2024-05-10T05:52:32Z

cli/src/config/misc.toml

@@ -13,7 +13,7 @@ allow-filesets = false
 always-allow-large-revsets = false
 diff-instructions = true
 paginate = "auto"
-pager = { command = ["less", "-FRX"], env = { LESSCHARSET = "utf-8" } }
+pager = { command = ["less"], env_default = { LESS="-FRX", LESSCHARSET = "utf-8" } }


Firstly, I think you don't need the - in LESS.

I'm not sure about Yuya's point. I have LESS set to FRX --mouse --wheel-lines=2, and I wouldn't like it overridden entirely. Perhaps we could print a warning after less quits if FRX was not in LESS and it was set? Perhaps we could prepend FRX to the variable?

martinvonz commented May 9, 2024

View reviewed changes

ilyagr reviewed May 10, 2024

View reviewed changes

cli/src/config/misc.toml Outdated Show resolved Hide resolved

martinvonz force-pushed the push-syxxkrvnzlox branch from 2f2836b to f195670 Compare May 10, 2024 05:28

martinvonz added 5 commits May 9, 2024 22:34

config: rewrite test using TOML input for readability

fdcce0c

cli: don't require env field in structured command config

cee473b

cli: allow non-overriding env vars for configured commands

7038f91

This lets you configure e.g. some environment variables to pass to a pager that will only be used if they're not already set in the user's environment. Thanks to @ilyagr for the suggestion.

cli: don't override $LESS and $LESSCHARSET

a965297

martinvonz force-pushed the push-syxxkrvnzlox branch from f195670 to aa675b9 Compare May 10, 2024 05:35

yuja reviewed May 10, 2024

View reviewed changes

ilyagr reviewed May 10, 2024

View reviewed changes

ilyagr mentioned this pull request Nov 3, 2024

CLI --help font or character format trouble when PAGER=less and LESS is unset (Alpine/Gentoo/NixOS default) #4745

Open

yuja mentioned this pull request Nov 12, 2024

cli: set LESS=FRX when running $PAGER #4834

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

config: set `$LESS` and `$LESSCHARSET` even if `$PAGER` is set #3657

config: set `$LESS` and `$LESSCHARSET` even if `$PAGER` is set #3657

martinvonz commented May 9, 2024

martinvonz May 9, 2024

yuja May 10, 2024

ilyagr May 10, 2024 •

edited

Loading

ilyagr May 10, 2024 •

edited

Loading

yuja May 10, 2024

ilyagr Nov 8, 2024 •

edited

Loading

ilyagr Nov 8, 2024 •

edited

Loading

ilyagr Nov 8, 2024 •

edited

Loading

ilyagr Nov 8, 2024

yuja Nov 8, 2024

yuja May 10, 2024

yuja May 10, 2024

ilyagr May 10, 2024

ilyagr May 10, 2024

ilyagr Nov 8, 2024

ilyagr May 10, 2024

ilyagr May 10, 2024

ilyagr May 10, 2024 •

edited

Loading

ilyagr May 10, 2024

config: set $LESS and $LESSCHARSET even if $PAGER is set #3657

Are you sure you want to change the base?

config: set $LESS and $LESSCHARSET even if $PAGER is set #3657

Conversation

martinvonz commented May 9, 2024

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilyagr May 10, 2024 • edited Loading

Choose a reason for hiding this comment

ilyagr May 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilyagr Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

ilyagr Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

ilyagr Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilyagr May 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

config: set `$LESS` and `$LESSCHARSET` even if `$PAGER` is set #3657

config: set `$LESS` and `$LESSCHARSET` even if `$PAGER` is set #3657

ilyagr May 10, 2024 •

edited

Loading

ilyagr May 10, 2024 •

edited

Loading

ilyagr Nov 8, 2024 •

edited

Loading

ilyagr Nov 8, 2024 •

edited

Loading

ilyagr Nov 8, 2024 •

edited

Loading

ilyagr May 10, 2024 •

edited

Loading